-
Notifications
You must be signed in to change notification settings - Fork 185
✨ feat(tests): EIP-7928 SELFDESTRUCT test #2159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
73e22e4 to
296091d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm overall, thanks for getting this in 👌🏼. I left some comments to think about but also, I don't expect selfdestruct tests to grow too large. I don't think these deserve their own file for this reason, please correct me if I'm wrong though. Perhaps we should move these into the positive (err... non-invalid) test cases for now instead of their own file. Thoughts?
tests/amsterdam/eip7928_block_level_access_lists/test_block_access_lists_self_destruct.py
Outdated
Show resolved
Hide resolved
tests/amsterdam/eip7928_block_level_access_lists/test_block_access_lists_self_destruct.py
Outdated
Show resolved
Hide resolved
tests/amsterdam/eip7928_block_level_access_lists/test_block_access_lists_self_destruct.py
Outdated
Show resolved
Hide resolved
tests/amsterdam/eip7928_block_level_access_lists/test_block_access_lists_self_destruct.py
Outdated
Show resolved
Hide resolved
tests/amsterdam/eip7928_block_level_access_lists/test_block_access_lists_self_destruct.py
Outdated
Show resolved
Hide resolved
tests/amsterdam/eip7928_block_level_access_lists/test_block_access_lists_self_destruct.py
Outdated
Show resolved
Hide resolved
296091d to
a3c2f1f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
re: creating a separate file for this test This is purely done to avoid conflicts while I work on these PRs in parallel (I have done the same in other PRs too). Once all PRs are merged, we could group them. Let me know if you want me to move them now. |
Oh I see. I wouldn't think it's difficult to rebase if these are standalone cases. It would be better to PR them to the appropriate place, but if that's messy I suppose that's fine. Don't let this be a blocker at all. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Just so you are in the loop @fselmo Self destructs will now include an address with |
|
@nerolation @raxhvl this should be updated in the EIP specifications, right? Anyone working on that? |
|
Yeah on it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We should move the test into test_block_access_lists.py before merging since self destruct cases shouldn't need a standalone file. I also updated the eels resolutions to point to the latest specs commit for Toni's changes that were merged in.
tests/amsterdam/eip7928_block_level_access_lists/test_block_access_lists_self_destruct.py
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
|
Will fix it - thanks! |
88370ff to
59b2dc7
Compare
Co-authored-by: felipe <[email protected]>
- Add a parametrized case for pre-funded selfdestruct account. In this case, the account was funded before the self-destruct, so we do record the balance post-state as being `0` since there is an actual balance change between pre-state and post-state. - The case where a same-transaction selfdestruct account was not pre-funded, there is no net balance change between pre-state and post-state so we don't record any balance change in the BAL.
7a01039 to
366e454
Compare
|
@raxhvl... @nerolation and I worked on self destruct back and forth some at the end of last week to get the specs ironed out and I added the specific case of the pre-funded self-destruct account where we want to record the balance change as I also pointed to the latest commit in the spec for the resolver. All the tests are currently filling here now. I think this is good to go? Let me know if we're missing anything 👀 |
|
Thanks @fselmo I made the following changes
@nerolation Here is the impact on specs
|
This comment was marked as resolved.
This comment was marked as resolved.
* Add more complex selfdestruct tests * Add tests for EIP-7928 around precompiles
I commented on the issue here, the tests here were not expecting the right things. This does fill appropriately now (fixed here. I think this PR is good to be merged. |
dae8940 to
81fcca4
Compare
81fcca4 to
ac1e471
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Couple small comments. No blockers. Feel free to merge :D
* ✨ feat(tests): EIP-7928 SELFDESTRUCT tests * feat: point to latest commit in BALs specs (resolver) * feat: Validate t8n BAL does not have duplicate entries for the same tx_index * 📄 docs: Changelog entry * chore: avoid extra fields in BAL classes, related to #2197 * Add tests for EIP-7928 around precompiles (doc) * fix(tests): Fix expectations for self-destruct tests --------- Co-authored-by: raxhvl <[email protected]> Co-authored-by: fselmo <[email protected]> Co-authored-by: Toni Wahrstätter <[email protected]>
🗒️ Description
Test case effects of
SELFDESTRUCTon EIP-7928.cc: @nerolation
bal_changesfor the self-destructing contract is captured too.Additional changes
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlinttype(scope):.